Skip to content

Migration of the cf-java-client dependency - #1907

Open
karrgov wants to merge 20 commits into
masterfrom
cf_java_client_poc
Open

karrgov wants to merge 20 commits into
masterfrom
cf_java_client_poc

Conversation

@karrgov

@karrgov karrgov commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Migration of the cf-java-client dependency

Comment thread multiapps-controller-client/src/main/java/module-info.java Outdated

@theghost5800 theghost5800 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of comments are repeatable or not but overall do the checks through all the code again if possible

Comment thread multiapps-controller-client/pom.xml
Comment thread multiapps-controller-client/pom.xml
Comment thread pom.xml Outdated

@theghost5800 theghost5800 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of comments are repeatable or not but overall do the checks through all the code again if possible

@karrgov
karrgov force-pushed the cf_java_client_poc branch from 19c69f8 to e7fa4e6 Compare October 13, 2026 05:37
@sonarqubecloud

Copy link
Copy Markdown

Comment thread pom.xml
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>
<version>${hamcrest.version}}</version>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the second '}' here

Comment on lines +229 to +235
private ExecutorService createUploadMonitorExecutor() {
ThreadFactory threadFactory = Thread.ofVirtual()
.name(Constants.UPLOAD_MONITOR_THREAD_NAME_PREFIX, 0)
.factory();

return Executors.newThreadPerTaskExecutor(threadFactory);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecutorService should be shut down as part of its owner's lifecycle. Here its created but it does not appear to be closed.

Comment on lines +17 to +19
public static final int MAX_CONCURRENT_PAGES = 4; //leaving it like that for testing purposes
public static final int SECOND_PAGE = 2;
public static final int DEFAULT_CONCURRENT_TASKS = 8; //this one can be a bit more, but first tests will be like that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these values be left for prod? If so remove the comments.

Comment on lines +201 to +207
if (e.getStatusCode() == HttpStatus.NOT_FOUND) {
// package was most probably already consumed by CF between the polls, meaning that since the build was already created from the package object, the package is not needed anymore
// and the cf blobstore garbage collector has deleted it
keepPolling = false;
continue;
}
throw e;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-404 throw runs on the background monitor thread, and it seems like the error gets swallowed. Should this call callback.onError(e) like the FAILED-status branch below?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants